Skip to content

fix(ode): validate ExponentialMap::integrate_attitude's timestep and … - #307

Open
naseem173 wants to merge 5 commits into
kmolan:mainfrom
naseem173:fix/exponential-map-validation
Open

fix(ode): validate ExponentialMap::integrate_attitude's timestep and …#307
naseem173 wants to merge 5 commits into
kmolan:mainfrom
naseem173:fix/exponential-map-validation

Conversation

@naseem173

Copy link
Copy Markdown

…rate

Fixes #302. attitude_step and attitude_step_with_angular_acceleration fed a timestep and angular rate straight into SO3::exp with no validation: a non-finite rate produced a quaternion of NaNs that never recovered, and a non-positive timestep integrated backwards (or not at all) without comment.

attitude_step and attitude_step_with_angular_acceleration stay infallible, since they sit on RigidBody::stepped's documented panic-free per-tick path (and estimation::attitude_correction's per-tick path) - making them fallible would force that hot loop to become fallible too. Their docs now carry an explicit "Behavior" note describing the NaN policy instead of leaving it silent.

integrate_attitude, the multi-step driver (not used on any per-tick hot path, and already the same shape as the fallible Rk45::solve), now validates dt up front and the rate returned by its angular_rate_at callback on every step, returning the new IntegrateError::NonPositiveTimestep or the existing IntegrateError::NonFinite instead of silently producing garbage.

What & why

Adds input validation to ExponentialMap::integrate_attitude per #302, without changing the infallible signature of the per-tick primitives it's built from. See the commit message above for the full reasoning on scope.

Checklist

  • cargo test + cargo clippy --all-targets clean locally (not run locally in this environment — relying on CI to verify; happy to fix any failures)
  • New public APIs have a doc example
  • No unwrap/expect/panic on library paths (typed errors instead)

…rate

Fixes kmolan#302. attitude_step and attitude_step_with_angular_acceleration fed a
timestep and angular rate straight into SO3::exp with no validation: a
non-finite rate produced a quaternion of NaNs that never recovered, and a
non-positive timestep integrated backwards (or not at all) without comment.

attitude_step and attitude_step_with_angular_acceleration stay infallible,
since they sit on RigidBody::stepped's documented panic-free per-tick path
(and estimation::attitude_correction's per-tick path) - making them fallible
would force that hot loop to become fallible too. Their docs now carry an
explicit "Behavior" note describing the NaN policy instead of leaving it
silent.

integrate_attitude, the multi-step driver (not used on any per-tick hot
path, and already the same shape as the fallible Rk45::solve), now validates
dt up front and the rate returned by its angular_rate_at callback on every
step, returning the new IntegrateError::NonPositiveTimestep or the existing
IntegrateError::NonFinite instead of silently producing garbage.
@naseem173
naseem173 requested a review from kmolan as a code owner August 9, 2026 21:02

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the first PR! 🎉 You'll get a first review within 48 h. CI runs the 5-target matrix — a budget-gate failure isn't your fault, see ci/README.md.

@kmolan

kmolan commented Aug 9, 2026

Copy link
Copy Markdown
Owner

@naseem173 thanks for the contribution, please address existing failures before I review the changes. Looks like you need to run cargo fmt --all and commit the formatting changes, plus whatever compilation error the CI pipelines are getting. Lmk if its unclear what to fix.

naseem173 and others added 2 commits August 10, 2026 14:18
- error.rs: fix indentation of the NonPositiveTimestep Display arm
- tests/suite/ode/exponential_map.rs: fix import order and split
  chained .unwrap() calls per rustfmt
  - tutorials/ode-integrators.md: add .unwrap() to the now-fallible
    integrate_attitude doctest call, and update the "Errors" paragraph
      to reflect that attitude_step/attitude_step_with_angular_acceleration
        stay infallible while integrate_attitude now returns IntegrateError
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@naseem173

Copy link
Copy Markdown
Author

Thanks for the review! I've run cargo fmt --all and pushed the formatting fixes (commit 65d7256) — the fmt + clippy diffs in tests/suite/ode/exponential_map.rs are resolved. The rest of the matrix was already green on the prior commit. The new CI run is currently showing action_required, so it looks like it's waiting on a maintainer approval to run. Let me know if anything else needs addressing!

Result is already #[must_use]; the extra attribute trips clippy::double_must_use under -D warnings.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@naseem173

Copy link
Copy Markdown
Author

Pushed a follow-up (commit 07aa5c4): fmt now passes, but the same job's cargo clippy -- -D warnings flagged a redundant #[must_use] on integrate_attitude (double_must_use, since it already returns a Result). I've removed that attribute — the other #[must_use]s in the file return SO3<T> and are fine. All other jobs (tests, bare-metal targets, docs, packaging) were green. Looks like the new run is awaiting approval again whenever you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate the timestep and rates in ExponentialMap

2 participants